Remove unused/unreachable code from ops #18964

jbrockmendel · 2017-12-28T05:57:15Z

Add TODO comments for questionable cases.

passes git diff upstream/master -u -- "*.py" | flake8 --diff

codecov · 2017-12-28T08:41:22Z

Codecov Report

Merging #18964 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18964      +/-   ##
==========================================
+ Coverage   91.57%   91.59%   +0.02%     
==========================================
  Files         150      150              
  Lines       48941    48909      -32     
==========================================
- Hits        44817    44798      -19     
+ Misses       4124     4111      -13

Flag	Coverage Δ
#multiple	`89.95% <100%> (+0.02%)`	⬆️
#single	`41.77% <90%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/ops.py	`93.94% <100%> (+3.7%)`	⬆️
pandas/plotting/_converter.py	`65.22% <0%> (-1.74%)`	⬇️
pandas/core/computation/expressions.py	`93.27% <0%> (-1.69%)`	⬇️
pandas/core/indexes/datetimelike.py	`97.04% <0%> (-0.02%)`	⬇️
pandas/util/testing.py	`84.95% <0%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c19bdc9...5b704fa. Read the comment docs.

jreback · 2017-12-28T11:02:09Z

pandas/core/ops.py

@@ -721,6 +686,10 @@ def safe_na_op(lvalues, rvalues):
                return na_op(lvalues, rvalues)
        except Exception:
            if isinstance(rvalues, ABCSeries):
+                # TODO: This case is not hit in tests.  For it to be hit would
+                # require `right` to be an object such that right.values


limit the comments to useful info, 'screwup' does not fall in this category

This comment is more a suggestion that we get rid of this block.

I get that. but this is public code, you can certainly say 'this should be removed'

ok with removing this code here

jreback · 2017-12-28T11:02:36Z

pandas/core/ops.py

@@ -731,7 +700,9 @@ def safe_na_op(lvalues, rvalues):
                                                  lambda x: op(x, rvalues))
            raise

-    def wrapper(left, right, name=name, na_op=na_op):
+    def wrapper(left, right, na_op=na_op):
+        # TODO: `na_op` does not belong in Series.__{op}__ signature.  But this


0.21.1:

>>> inspect.getargspec(pd.Series.__add__) ArgSpec(args=['left', 'right', 'name', 'na_op'], varargs=None, keywords=None, defaults=('__add__', <function na_op at 0x10c4b7d70>))

and so what?

... and there's precedent for caring about signatures that make sense, especially in super-commonly-used user-facing methods.

Don't worry about it anyway, I found a way to get na_op and name out of the signature.

jreback · 2017-12-28T11:04:07Z

pandas/core/ops.py


+    wrapper.__name__ = name


I believe this is hit in tests, yes? (the name of the wrapper)

0.21.1:

>>> pd.Series.__sub__ <unbound method Series.wrapper>

hmm yeah that's not right. can you add tests for this? (I believe we have a suite of tests for the stat methods, so prob just need to add these on).

We recently made similar tests for Index comparison method names in tests.indexes.test_base, I don't see much else to that effect. I'll put something together.

https://github.com/pandas-dev/pandas/blob/master/pandas/tests/generic/test_generic.py#L507

jreback · 2017-12-28T11:04:39Z

pandas/core/ops.py

-    def wrapper(left, right, name=name, na_op=na_op):
+    def wrapper(left, right, na_op=na_op):
+        # TODO: `na_op` does not belong in Series.__{op}__ signature.  But this
+        # is needed here for closure/namespace purposes ATM.


can you add doc-strings. this is extremely useful to future readers.

Any docstring added here would become the docstring for pd.Series.__foo__, which I doubt is what you have in mind here.

right, so just a comment then which describes the input & types

jreback · 2017-12-28T11:05:19Z

pandas/core/ops.py

@@ -1371,23 +1339,6 @@ def f(self, other):

 def _arith_method_PANEL(op, name, str_rep=None, fill_zeros=None,
                        default_axis=None, **eval_kwargs):
-    # copied from Series na_op above, but without unnecessary branch for
-    # non-scalar


ok, not super concerned about Panel things generally.

jreback · 2017-12-28T11:06:45Z

pandas/core/ops.py

    def _validate(self, lvalues, rvalues, name):
        if self.is_datetime_lhs:
            return self._validate_datetime(lvalues, rvalues, name)
        elif self.is_timedelta_lhs:
            return self._validate_timedelta(name)
-        elif self.is_offset_lhs:


why is this offset case not possible? (it may not be tested) but is it possible?

Because _TimeOp is only used if is_datetime_lhs or is_timedelta_lhs

ok, can you doc-string things a bit, will help future readers.

…s_cleanup

pep8speaks · 2017-12-29T06:12:59Z

Hello @jbrockmendel! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 01, 2018 at 16:47 Hours UTC

…s_cleanup

jreback · 2017-12-31T00:56:00Z

pandas/core/ops.py


+    wrapper.__name__ = name
+    if hasattr(operator, name) and callable(getattr(operator, name)):


huh? what is this for

You asked for generic docstrings for these ops. These are about as generic as they get. But there is no e.g. operator.rfloordiv, so we have to condition.

I understand, but this is an obscure way of doing this. I would rather actually construct a proper doc-string here

OK, I'll remove this, can consider docstrings out of scope for this PR.

jreback · 2017-12-31T15:05:04Z

pandas/core/ops.py

@@ -354,24 +356,25 @@ class _TimeOp(_Op):
    """
    Wrapper around Series datetime/time/timedelta arithmetic operations.
    Generally, you should use classmethod ``_Op.get_op`` as an entry point.
+
+    This is only reached in cases where either `self.is_datetime_lhs`


you don't need the single back ticks

jreback · 2017-12-31T15:06:54Z

pandas/core/ops.py

        self.is_timedelta_lhs = is_timedelta64_dtype(lvalues)
        self.is_datetime64_lhs = is_datetime64_dtype(lvalues)
        self.is_datetime64tz_lhs = is_datetime64tz_dtype(lvalues)
        self.is_datetime_lhs = (self.is_datetime64_lhs or
                                self.is_datetime64tz_lhs)
-        self.is_integer_lhs = left.dtype.kind in ['i', 'u']
-        self.is_floating_lhs = left.dtype.kind == 'f'
+        assert left.dtype.kind not in ['i', 'u', 'f'], left


assert one of the is_* e.g.
asert self.is_timedelta_lhs or self is_datetime_lhs

jreback · 2017-12-31T15:07:39Z

pandas/core/ops.py

@@ -517,15 +489,16 @@ def _convert_to_array(self, values, name=None, other=None):
            elif isinstance(values, pd.DatetimeIndex):
                values = values.to_series()
            # datetime with tz


fix this comment

jreback · 2017-12-31T15:10:47Z

pandas/core/ops.py

                values = pd.DatetimeIndex(values)
            # datetime array with tz
            elif is_datetimetz(values):
                if isinstance(values, ABCSeries):
                    values = values._values
            elif not (isinstance(values, (np.ndarray, ABCSeries)) and
                      is_datetime64_dtype(values)):
+                # TODO: This is not hit in tests.  What case is it intended


I think can remove this case if change 492 to
elif isinstance(ovalues, (datetime.datetime, np.datetime64))

In [4]: np.datetime64('2013-01-01T00:00:00.000000000') Out[4]: numpy.datetime64('2013-01-01T00:00:00.000000000') In [5]: isinstance(np.datetime64('2013-01-01T00:00:00.000000000'), np.ndarray) Out[5]: False In [6]: isinstance(np.array(np.datetime64('2013-01-01T00:00:00.000000000')), np.ndarray) Out[6]: True In [7]: np.array(np.datetime64('2013-01-01T00:00:00.000000000')) Out[7]: array(1356998400000000000, dtype='datetime64[ns]')

Sounds good.

jreback · 2017-12-31T15:11:33Z

pandas/core/ops.py

@@ -721,6 +686,10 @@ def safe_na_op(lvalues, rvalues):
                return na_op(lvalues, rvalues)
        except Exception:
            if isinstance(rvalues, ABCSeries):
+                # TODO: This case is not hit in tests.  For it to be hit would
+                # require `right` to be an object such that right.values


ok with removing this code here

jreback · 2017-12-31T15:12:40Z

pandas/core/ops.py


+    wrapper.__name__ = name
+    if hasattr(operator, name) and callable(getattr(operator, name)):


I understand, but this is an obscure way of doing this. I would rather actually construct a proper doc-string here

jreback · 2017-12-31T15:13:07Z

pandas/core/ops.py

@@ -857,6 +826,8 @@ def wrapper(self, other, axis=None):
        # Validate the axis parameter
        if axis is not None:
            self._get_axis_number(axis)


yes should change (but agreed may not be hit in tests)

…s_cleanup

jreback · 2018-01-01T17:55:42Z

closing in favor of #19024

remove unused code, impossible cases. add todo comments

65dd8b3

jreback requested changes Dec 28, 2017

View reviewed changes

jreback added the Clean label Dec 28, 2017

jreback reviewed Dec 28, 2017

View reviewed changes

jbrockmendel added 5 commits December 28, 2017 20:35

add tests for op names

f65eab4

Merge branch 'master' of https://github.com/pandas-dev/pandas into op…

9091386

…s_cleanup

trim the set of tested funcnames

5a5d34a

troubleshooting, add tests that belong elsewhere

38d6c08

fix Series op signatures

c712bfc

jbrockmendel added 5 commits December 28, 2017 22:22

cleanup commented-out bits from troubleshooting

4a2a021

cleanup leftover troubleshooting relics

e53b7fa

Merge branch 'master' of https://github.com/pandas-dev/pandas into op…

13ebe0a

…s_cleanup

Merge branch 'master' of https://github.com/pandas-dev/pandas into op…

f37eb9d

…s_cleanup

Merge branch 'master' of https://github.com/pandas-dev/pandas into op…

58f7ef7

…s_cleanup

jreback requested changes Dec 31, 2017

View reviewed changes

jreback added Timedelta Timedelta data type Datetime Datetime data dtype labels Dec 31, 2017

jbrockmendel added 2 commits December 31, 2017 09:43

Merge branch 'master' of https://github.com/pandas-dev/pandas into op…

d8305c1

…s_cleanup

requested edits

fdf461e

jbrockmendel mentioned this pull request Jan 1, 2018

dispatch Series[datetime64] ops to DatetimeIndex #19024

Merged

4 tasks

Merge branch 'master' of https://github.com/pandas-dev/pandas into op…

5b704fa

…s_cleanup

jreback closed this Jan 1, 2018

jreback added this to the No action milestone Jan 4, 2018

jbrockmendel deleted the ops_cleanup branch January 23, 2018 04:40


		wrapper.__name__ = name
		if hasattr(operator, name) and callable(getattr(operator, name)):

Remove unused/unreachable code from ops #18964

Remove unused/unreachable code from ops #18964

Conversation

jbrockmendel commented Dec 28, 2017

codecov bot commented Dec 28, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Dec 29, 2017 • edited Loading

Comment last updated on January 01, 2018 at 16:47 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 1, 2018

codecov bot commented Dec 28, 2017 •

edited

Loading

jbrockmendel Dec 28, 2017 •

edited

Loading

pep8speaks commented Dec 29, 2017 •

edited

Loading